Skip to content

refactor: reuse calculated fees#1462

Merged
mergify[bot] merged 5 commits into
anza-xyz:masterfrom
jstarry:refactor/pass-fee
May 29, 2024
Merged

refactor: reuse calculated fees#1462
mergify[bot] merged 5 commits into
anza-xyz:masterfrom
jstarry:refactor/pass-fee

Conversation

@jstarry
Copy link
Copy Markdown

@jstarry jstarry commented May 22, 2024

Problem

Transaction fees are recalculated quite a few times in the transaction pipeline for no good reason

Summary of Changes

Store calculated fee details in LoadedTransaction and TransactionExecutionDetails so that fees don't need to be recalculated downstream

Fixes #

@jstarry jstarry force-pushed the refactor/pass-fee branch from 8ca3036 to c6e0942 Compare May 22, 2024 23:21
@jstarry jstarry force-pushed the refactor/pass-fee branch from c6e0942 to ae5ca7b Compare May 24, 2024 04:17
@jstarry jstarry marked this pull request as ready for review May 24, 2024 04:22
@jstarry jstarry requested review from apfitzge and tao-stones May 24, 2024 04:22
@apfitzge
Copy link
Copy Markdown

Just looking at title it seems effort is similar to Tao's tx meta.
Might wanna coordinate with him or get his thoughts on this.

@jstarry jstarry force-pushed the refactor/pass-fee branch from ae5ca7b to 3ff403c Compare May 24, 2024 13:55
@tao-stones
Copy link
Copy Markdown

Cached FeeDetails should be invalidated between epoch boundary, should it? Inputs to calculate_fee_details() such as budget_limits are often feature gates dependent, so does calculation itself.

One version of solana_runtime_transaction::transaction_meta set ttl to cached data with get_last_slot_in_epoch(); While it helped to reuse cached value within epoch, the interface was rather messy: getter needs to take current_slot as additional parameter, and returns Option<>. In case of None returned (crossed epoch), callsite needs to find all necessary inputs to refresh the cached value. Is there better way?

@jstarry
Copy link
Copy Markdown
Author

jstarry commented May 24, 2024

Epoch boundaries aren't relevant to this PR refactoring because the fee that this PR is reusing is calculated during transaction loading / execution in the SVM. Epoch boundaries are only relevant when scheduling transactions in banking stage. This PR doesn't change any behavior in the banking stage and so I don't think this will conflict at all with tx meta work that @tao-stones is working on

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.7%. Comparing base (a4a009e) to head (abe1f46).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1462   +/-   ##
=======================================
  Coverage    82.7%    82.7%           
=======================================
  Files         876      876           
  Lines      371118   371045   -73     
=======================================
- Hits       307263   307212   -51     
+ Misses      63855    63833   -22     

@jstarry jstarry force-pushed the refactor/pass-fee branch from 3ff403c to abe1f46 Compare May 26, 2024 17:12
Copy link
Copy Markdown

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree caching FeeDetails at LoadedTransaction makes a lot sense in this PR - bank is going to collect exact fee that account_loader calculated and validated against payer account. My early commit is due to the concern that dev could accidentally cache and reuse LoadedTransaction (at banking stage) cross epoch boundary, even it's not intentional. Perhaps that's overly concerned? Other than that, the PR looks great.

Comment thread svm/src/account_loader.rs Outdated
Comment thread sdk/src/fee.rs
@jstarry
Copy link
Copy Markdown
Author

jstarry commented May 29, 2024

My early comment is due to the concern that dev could accidentally cache and reuse LoadedTransaction (at banking stage) cross epoch boundary, even it's not intentional. Perhaps that's overly concerned?

Yeah, I don't think it's a valid concern because basically nothing in LoadedTransaction should be loaded during one bank and then used in a new bank, let alone across an epoch boundary.

tao-stones
tao-stones previously approved these changes May 29, 2024
Copy link
Copy Markdown

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - thanks for clarification.

@jstarry jstarry added the automerge automerge Merge this Pull Request automatically once CI passes label May 29, 2024
@mergify mergify Bot removed the automerge automerge Merge this Pull Request automatically once CI passes label May 29, 2024
@mergify
Copy link
Copy Markdown

mergify Bot commented May 29, 2024

automerge label removed due to a CI failure

@jstarry jstarry added the automerge automerge Merge this Pull Request automatically once CI passes label May 29, 2024
@mergify mergify Bot merged commit 2cc540a into anza-xyz:master May 29, 2024
@jstarry jstarry deleted the refactor/pass-fee branch May 29, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants